Skip to content
This repository has been archived by the owner on Feb 24, 2025. It is now read-only.

Exclude child binaries #3824

Merged
merged 18 commits into from
Feb 7, 2025
Merged

Exclude child binaries #3824

merged 18 commits into from
Feb 7, 2025

Conversation

diegoreymendez
Copy link
Contributor

@diegoreymendez diegoreymendez commented Feb 4, 2025 •
edited
Loading

Task/Issue URL: https://app.asana.com/0/1206580121312550/1209309062840626/f

Description

See Figma for reference (keep in mind this is a Windows Figma, and there's no macOS one).

Changes:

  • When a routing-rule is applied to an app through the VPN, its embedded binaries will be subjected to the same rules.

Testing

Test Exclusions

  1. Enable App Exclusions in: Debug Menu > Feature Flag Overrides > networkProtectionAppExclusions
  2. Start the VPN
  3. Exclude Chrome
  4. Make sure the web traffic is excluded from the VPN. This is handled by a helper.

Definition of Done:

Internal references:

Pull Request Review Checklist
Software Engineering Expectations
Technical Design Template
Pull Request Documentation

@diegoreymendez diegoreymendez marked this pull request as ready for review February 4, 2025 13:45
@diegoreymendez diegoreymendez changed the base branch from main to diego/exclusions-in-vpn-status-view February 4, 2025 13:46
@diegoreymendez diegoreymendez self-assigned this Feb 4, 2025
Base automatically changed from diego/exclusions-in-vpn-status-view to main February 4, 2025 20:54
@diegoreymendez diegoreymendez changed the title Excluded child binaries Exclude child binaries & update VPN menu Feb 6, 2025
alessandroboron
alessandroboron approved these changes Feb 6, 2025
View reviewed changes
Copy link
Contributor

@alessandroboron alessandroboron left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It works as expected! I’m approving it as I don’t see issue in the code. Some suggestions:

  • Would add Xcode inline markdown documentation for public methods defined in packages to help future developers.
  • I noticed there are no tests added. I’m not sure if it can’t be done but it would be good to have test coverage on the VPN feature.

@diegoreymendez diegoreymendez changed the title Exclude child binaries & update VPN menu Exclude child binaries Feb 6, 2025
@diegoreymendez
Copy link
Contributor Author

Thanks @alessandroboron !

  • I've added documentation to AppInfoRetriever.
  • I've also improved some of the code I was pushing to inject the retriever using the protocol instead of instantiating it directly.
  • Regarding the tests it's a bit tricky because a lot of this feature depends on multiple processes. There's space to add unit tests in the AppInfoRetriever module but I'll push that in a follow-up PR (the one I'll send to ship review).

@diegoreymendez diegoreymendez merged commit 347b682 into main Feb 7, 2025
29 checks passed
@diegoreymendez diegoreymendez deleted the diego/app-exclusions-support-embedded-apps branch February 7, 2025 09:18
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@alessandroboron alessandroboron alessandroboron approved these changes

@samsymons samsymons Awaiting requested review from samsymons

Assignees

@diegoreymendez diegoreymendez

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@diegoreymendez @alessandroboron